refactor: use HTTP status code descriptions in files plugin#258
refactor: use HTTP status code descriptions in files plugin#258atilafassina merged 3 commits intoreq-error-handlingfrom
Conversation
…r strings Replace hardcoded error messages like "List failed" with standard HTTP status descriptions from node:http STATUS_CODES for consistent error responses. Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
There was a problem hiding this comment.
Pull request overview
Updates the Files plugin to return standardized error messages derived from Node’s HTTP status descriptions, and aligns test expectations with the new error strings.
Changes:
- Use
node:httpSTATUS_CODESto populate{ error }responses based on the returned HTTP status. - Replace several hardcoded
"X failed"error strings with status descriptions (e.g.,500 -> "Internal Server Error"). - Update unit + integration tests to assert the new standardized error messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/appkit/src/plugins/files/plugin.ts |
Replaces several error responses with STATUS_CODES[status]-derived messages. |
packages/appkit/src/plugins/files/tests/plugin.test.ts |
Updates unit test assertions to expect "Internal Server Error" instead of hardcoded strings. |
packages/appkit/src/plugins/files/tests/plugin.integration.test.ts |
Updates integration test assertions to expect standardized status descriptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!result.ok) { | ||
| res | ||
| .status(result.status) | ||
| .json({ error: "List failed", plugin: this.name }); | ||
| .json({ | ||
| error: STATUS_CODES[result.status] ?? "Unknown Error", | ||
| plugin: this.name, | ||
| }); |
There was a problem hiding this comment.
The status-to-message mapping logic is duplicated across multiple handlers (list/read/exists/metadata/preview/upload/mkdir/delete). Consider extracting a small helper (e.g., this._statusDescription(status) or this._sendStatusError(res, status)) to avoid repetition and ensure consistent behavior (including what to do for unknown/non-standard status codes).
| if (!response.ok) { | ||
| res | ||
| .status(response.status) | ||
| .json({ error: `${label} failed`, plugin: this.name }); | ||
| .json({ | ||
| error: STATUS_CODES[response.status] ?? "Unknown Error", | ||
| plugin: this.name, | ||
| }); |
There was a problem hiding this comment.
This handler now returns STATUS_CODES[response.status] for !response.ok, but there are still 500 error responses in the same flow that use hardcoded messages (e.g., the stream error event uses ${label} failed). If the goal is to standardize error strings to HTTP status descriptions, please update those remaining paths (and/or route everything through a shared error helper) so clients don’t see mixed error formats for the same status code.
Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
Addresses Copilot review comment about mixed error formats in the read/download stream handler. Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
Summary
node:httpSTATUS_CODESTest plan
This pull request was AI-assisted by Isaac.